Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browser extension signing example #1067

Merged

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Jul 13, 2023

I split the Yew WASM App into routes for the 2 examples.
This example constructs a System->remark call with a remark parsed from user input.
We then let the user choose an account in the Talisman or PolkadotJS browser extension and sign the extrinsic in the extension. (Currently tested with the Dev version of the polkadotjs extension)
The full SubmittableExtrinsic is then sent to a local node where we wait for it to be successful.

image
image
image
image
image
image

What is missing?
Right now era, tip and mortality_checkpoint of BaseExtrinsicParams are not used for the extrinsic and we use hardcoded values instead (no tip and immortal)

tadeohepperle and others added 16 commits May 26, 2023 14:37
* Reduce some repetition when obtaining metadata pallets/runtime_traits

* make them pub

* fix docs and clippy
Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.28.1 to 1.28.2.
- [Release notes](https://github.com/tokio-rs/tokio/releases)
- [Commits](tokio-rs/tokio@tokio-1.28.1...tokio-1.28.2)

---
updated-dependencies:
- dependency-name: tokio
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [regex](https://github.com/rust-lang/regex) from 1.8.2 to 1.8.3.
- [Release notes](https://github.com/rust-lang/regex/releases)
- [Changelog](https://github.com/rust-lang/regex/blob/master/CHANGELOG.md)
- [Commits](rust-lang/regex@1.8.2...1.8.3)

---
updated-dependencies:
- dependency-name: regex
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [quote](https://github.com/dtolnay/quote) from 1.0.27 to 1.0.28.
- [Release notes](https://github.com/dtolnay/quote/releases)
- [Commits](dtolnay/quote@1.0.27...1.0.28)

---
updated-dependencies:
- dependency-name: quote
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [proc-macro2](https://github.com/dtolnay/proc-macro2) from 1.0.58 to 1.0.59.
- [Release notes](https://github.com/dtolnay/proc-macro2/releases)
- [Commits](dtolnay/proc-macro2@1.0.58...1.0.59)

---
updated-dependencies:
- dependency-name: proc-macro2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@tadeohepperle tadeohepperle requested a review from a team as a code owner July 13, 2023 16:15
pub enum Route {
#[at("/fetching")]
Fetching,
#[at("/signing")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yew is quite nice really; I like the built in browser history/routing stuff, and actually I generally find it quite easy to follow overall!

I wonder how the other frameworks differ; I should really try some of them at some point!

wasm-bindgen-futures = "0.4.36"
anyhow = "1.0.71"
serde = "1.0.163"
serde_json = "1.0.96"
Copy link
Collaborator

@lexnv lexnv Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could add a new line here and below

@@ -0,0 +1,92 @@
/// The `@polkadot/extension-dapp` package can be dynamically imported.
/// Usually it is wise to use a package manager like npm or yarn to install it as a dependency.
/// The `getPolkadotJsExtensionMod`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with js docs, but is this line complete here? Or should we remove it?

transaction_version: u32,
genesis_hash: T::Hash,
mortality_checkpoint: T::Hash,
/// era
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// era
/// Era defines how long the transaction will be valid for.

pub transaction_version: u32,
/// genesis hash of the chain
pub genesis_hash: T::Hash,
/// mortality checkpoint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// mortality checkpoint
/// The block after which the transaction becomes valid.

pub era: Era,
/// nonce (account nonce sent along an extrinsic, such that no extrinsic is submitted twice)
pub nonce: u64,
/// tip
Copy link
Collaborator

@lexnv lexnv Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// tip
/// The tip you would like to give to the block author for this transaction.
/// Note: Can be zero.

@@ -318,7 +318,8 @@ where
pub struct PartialExtrinsic<T: Config, C> {
client: C,
call_data: Vec<u8>,
additional_and_extra_params: T::ExtrinsicParams,
/// Extrinsic params (Additional and Extra)
pub additional_and_extra_params: T::ExtrinsicParams,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Similarly to call_data we could create a getter for these params

/// Returns the additional and extra extrinsic params.
pub fn extra_params(&self) -> &T::ExtrinsicParams { .. }

@jsdw
Copy link
Collaborator

jsdw commented Jul 31, 2023

Looks great to me, and it works well!

I'd prefer that we don't expose any of those extra props in Subxt, and instead gather the information up from elsewhere; see my above comment :)

Comment on lines +126 to +130
fn encode_to_hex_reverse<E: Encode>(input: &E) -> String {
let mut bytes = input.encode();
bytes.reverse();
format!("0x{}", hex::encode(bytes))
}
Copy link
Collaborator

@jsdw jsdw Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's really because we aren't scale encoding some of these numbers at all; we're just providing the hex representation of them, which is naturally big endian. So really we should just hex::encode the numbers when converted to big endian bytes.

Copy link
Collaborator

@jsdw jsdw Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the functions should be:

// SCALE encode and then hex:
fn scale_encoded_hex<E: Encode>(input: &E) -> String { ... }
// Just hex encode the provided bytes:
fn hex_encoded_bytes(input: &[u8]) -> String { ... }

And then .to_be_bytes or whatever on the numbers we pass to the latter?

///
/// Some parameters are hard-coded here and not taken from the partial_extrinsic itself (mortality_checkpoint, era, tip).
pub async fn extension_signature_for_partial_extrinsic(
partial_extrinsic: &PartialExtrinsic<PolkadotConfig, OnlineClient<PolkadotConfig>>,
Copy link
Collaborator

@jsdw jsdw Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you could just pass in the api.tx().call_data(remark_call) bytes here I think; you don't use any other part of the PartialExtrinsic :)

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor nits but assuming it still works when tested again, it looks great to me!

@tadeohepperle tadeohepperle merged commit dc0aeac into master Aug 2, 2023
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-browser-extension-signing-example branch August 2, 2023 12:56
@jsdw jsdw mentioned this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants